Reconstruct `ChannelManager` forwarded HTLCs maps from `Channel`s #4227

valentinewallace · 2025-11-17T23:33:50Z

We have an overarching goal of (mostly) getting rid of ChannelManager persistence and rebuilding the ChannelManager's state from existing ChannelMonitors, due to issues when the two structs are out-of-sync on restart. The main issue that can arise is channel force closure.

Here we start this process by rebuilding ChannelManager::decode_update_add_htlcs, forward_htlcs, and pending_intercepted_htlcs from the Channels, which will soon be included in the ChannelMonitors as part of #4218.

test upgrading from a manager containing pending HTLC forwards that was serialized on <= LDK 0.2, i.e. where Channels will not contain committed update_add_htlcs
currently, no tests fail when we force using the new rebuilt decode_update_add_htlcs map and ignoring the legacy maps. This may indicate missing test coverage, since in theory we need to re-forward these HTLCs sometimes so they go back in the forward_htlcs map for processing
only use the old legacy maps if the manager and its channels were last serialized on <= 0.2. Currently this is not guaranteed

ldk-reviews-bot · 2025-11-17T23:33:52Z

👋 Thanks for assigning @joostjager as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-11-18T00:49:14Z

Codecov Report

❌ Patch coverage is 88.16794% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.33%. Comparing base (de384ff) to head (dfef41a).

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	86.92%	15 Missing and 5 partials ⚠️
lightning/src/ln/channel.rs	80.00%	9 Missing and 1 partial ⚠️
lightning/src/ln/reload_tests.rs	98.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4227      +/-   ##
==========================================
- Coverage   89.33%   89.33%   -0.01%     
==========================================
  Files         180      180              
  Lines      139042   139248     +206     
  Branches   139042   139248     +206     
==========================================
+ Hits       124219   124403     +184     
- Misses      12196    12218      +22     
  Partials     2627     2627

Flag	Coverage Δ
fuzzing	`35.93% <21.28%> (-0.04%)`	⬇️
tests	`88.70% <88.16%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

lightning/src/ln/channelmanager.rs

valentinewallace · 2025-11-19T17:24:30Z

@joostjager helped me realize this may be way overcomplicated, essentially all tests pass on main when we simply read-and-discard the pending forwards maps. It's a bit suspicious though that all tests pass so it seems like additional test coverage could be useful.

Nvm, our test coverage for reload of these maps is just pretty incomplete.

valentinewallace · 2025-11-19T22:03:46Z

Updated with new testing and a few tweaks: diff

Will rebase next

joostjager

Did an initial review pass. Even though the change is pretty contained, I still found it difficult to fully follow what's happening.

Overall I think comments are very much on the light side in LDK, and the code areas touched in this PR are no exception in my opinion. Maybe, now that you've invested the time to build understanding, you can liberally sprinkle comments on your changes and nearby code?

lightning/src/ln/channel.rs

joostjager · 2025-11-20T07:58:12Z

lightning/src/ln/channel.rs

 			payment_hash: PaymentHash(Sha256::hash(&[42; 32]).to_byte_array()),
 			cltv_expiry: 300000000,
-			state: InboundHTLCState::Committed,
+			state: InboundHTLCState::Committed { update_add_htlc_opt: None },


Is it ok to never populate update_add_htlc_opt in the tests in this commit, if that isn't supposed to happen in prod?

Yeah, because that field is only used by the manager on restart. Hopefully it's clearer with docs are on the field.

I thought maybe the copying of the update_add message into InboundHTLCState needs to be covered in this commit. It isn't currently. But coverage is probably obtained later on.

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

joostjager · 2025-11-20T08:26:12Z

lightning/src/ln/reload_tests.rs

+	args_b_c.send_announcement_sigs = (true, true);
+	reconnect_nodes(args_b_c);
+
+	// Forward the HTLC and ensure we can claim it post-reload.


What would happen again without the changes in this PR? I assume the htlc wouldn't be forwarded here, but would it be failed back instead?

You mean if we did the 3-line fix discussed offline? I don't think so, because the manager will only fail HTLCs that it knows about. I think we just wouldn't handle the HTLC and the channel would FC.

Yes indeed, if we just discard the forward htlcs. I thought there was still some inbound htlc timer somewhere, but I guess not then?

I wonder how fixing the FC by failing back the htlc and then completely forgetting about forwards on restart would compare to the current approach.

Hmm... seems like a significant UX downgrade for routing nodes?

But is it significant? Restart is probably rare on a routing node, and then also doing it right when a forward is being processed is even more rare?

Similar to the other /target directories we ignore where a bunch of files are generated during testing.

valentinewallace · 2025-12-02T00:52:40Z

I still need to finish adding some comments and responding to feedback, but I believe the tests are done and this should be mostly there so pushed up the latest state.

tnull · 2025-12-02T11:35:58Z

I still need to finish adding some comments and responding to feedback, but I believe the tests are done and this should be mostly there so pushed up the latest state.

We had discussed whether to split out receive_htlcs as part of this work, since you're already reconstructing the forwards_htlcs map. Are you still considering doing this as part of this PR or rather not?

valentinewallace · 2025-12-02T23:05:10Z

I still need to finish adding some comments and responding to feedback, but I believe the tests are done and this should be mostly there so pushed up the latest state.

We had discussed whether to split out receive_htlcs as part of this work, since you're already reconstructing the forwards_htlcs map. Are you still considering doing this as part of this PR or rather not?

Probably not in this PR since it's already decently sized and has a clear scope, but I will look into it for follow-up!

We have an overarching goal of (mostly) getting rid of ChannelManager persistence and rebuilding the ChannelManager's state from existing ChannelMonitors, due to issues when the two structs are out-of-sync on restart. The main issue that can arise is channel force closure. As part of this, we plan to store at least parts of Channels in ChannelMonitors, and that Channel data will be used in rebuilding the manager. Once we store update_adds in Channels, we can use them on restart when reconstructing ChannelManager maps such as forward_htlcs and pending_intercepted_htlcs. Upcoming commits will start doing this reconstruction.

We have an overarching goal of (mostly) getting rid of ChannelManager persistence and rebuilding the ChannelManager's state from existing ChannelMonitors, due to issues when the two structs are out-of-sync on restart. The main issue that can arise is channel force closure. As part of rebuilding ChannelManager forward HTLCs maps, we will also add a fix that will regenerate HTLCIntercepted events for HTLC intercepts that are present but have no corresponding event in the queue. That fix will use this new method.

tnull · 2025-12-03T10:02:56Z

I still need to finish adding some comments and responding to feedback, but I believe the tests are done and this should be mostly there so pushed up the latest state.

We had discussed whether to split out receive_htlcs as part of this work, since you're already reconstructing the forwards_htlcs map. Are you still considering doing this as part of this PR or rather not?

Probably not in this PR since it's already decently sized and has a clear scope, but I will look into it for follow-up!

SGTM!

tnull · 2025-12-03T11:00:16Z

Btw, feel free to ping me whenever you think this is ready for a secondary reviewer!

joostjager

I like the new commit structure. Left a few remarks and questions, some probably due to a few pieces of understanding missing in my mental model.

joostjager · 2025-12-03T15:35:53Z

lightning/src/ln/channel.rs

+											// received on an old pre-0.1 version of LDK. In this case, we can't set
+											// `update_add_htlc_opt` here and need to rely on the HTLC being present in
+											// the legacy `ChannelManager` maps instead for it to be handled properly, if
+											// the HTLC is still around the next time we restart.


But if the HTLC isn't around, I don't think this is making the situation worse?

Can you clarify what you mean, or a suggested phrasing?

joostjager · 2025-12-03T15:46:43Z

lightning/src/ln/channel.rs

 			payment_hash: PaymentHash(Sha256::hash(&[42; 32]).to_byte_array()),
 			cltv_expiry: 300000000,
-			state: InboundHTLCState::Committed,
+			state: InboundHTLCState::Committed { update_add_htlc_opt: None },


I thought maybe the copying of the update_add message into InboundHTLCState needs to be covered in this commit. It isn't currently. But coverage is probably obtained later on.

lightning/src/ln/channelmanager.rs

joostjager · 2025-12-03T16:09:07Z

lightning/src/ln/channelmanager.rs

+								// Reconstruct `ChannelManager::decode_update_add_htlcs` from the serialized
+								// `Channel`, as part of removing the requirement to regularly persist the
+								// `ChannelManager`.
+								decode_update_add_htlcs.insert(


Can't this be done in the next loop over the monitors where the update_add_htlcs are deduped? This loop seems to be about payments.

Hmm, IMO the first loop is about repopulating HTLC maps from monitors, and the second loop is about pruning maps from monitors (hence having two loops so we don't prune before the maps are completely filled out), so this approach seems to fit.

lightning/src/ln/channelmanager.rs

lightning/src/ln/functional_test_utils.rs

joostjager · 2025-12-03T16:26:15Z

lightning/src/ln/channelmanager.rs


 			forward_htlcs: Mutex::new(forward_htlcs_legacy),
-			decode_update_add_htlcs: Mutex::new(decode_update_add_htlcs_legacy),
+			decode_update_add_htlcs: Mutex::new(decode_update_add_htlcs),


I looked at the final commit of this PR, and there it seems that decode_update_add_htlcs_legacy isn't used at all anymore. Shouldn't it still be input in case we don't have data from monitor(s)? Maybe I am overlooking something.

Good catch, I fixed this and added a test.

lightning/src/ln/channelmanager.rs

ldk-reviews-bot · 2025-12-03T16:36:35Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

We have an overarching goal of (mostly) getting rid of ChannelManager persistence and rebuilding the ChannelManager's state from existing ChannelMonitors, due to issues when the two structs are out-of-sync on restart. The main issue that can arise is channel force closure. We'll use this new util when reconstructing the ChannelManager::decode_update_add_htlcs map from Channel data in upcoming commits. While the Channel data is not included in the monitors yet, it will be in future work.

We have an overarching goal of (mostly) getting rid of ChannelManager persistence and rebuilding the ChannelManager's state from existing ChannelMonitors, due to issues when the two structs are out-of-sync on restart. The main issue that can arise is channel force closure. Soon we'll be reconstructing these now-legacy maps from Channel data (that will also be included in ChannelMonitors in future work), so rename them as part of moving towards not needing to persist them in ChannelManager.

Makes an upcoming commit cleaner

We have an overarching goal of (mostly) getting rid of ChannelManager persistence and rebuilding the ChannelManager's state from existing ChannelMonitors, due to issues when the two structs are out-of-sync on restart. The main issue that can arise is channel force closure. Here we start this process by rebuilding ChannelManager::decode_update_add_htlcs from the Channels, which will soon be included in the ChannelMonitors as part of a different series of PRs. The newly built map is not yet used but will be in the next commit.

We have an overarching goal of (mostly) getting rid of ChannelManager persistence and rebuilding the ChannelManager's state from existing ChannelMonitors, due to issues when the two structs are out-of-sync on restart. The main issue that can arise is channel force closure. Here we start this process by rebuilding ChannelManager::decode_update_add_htlcs, forward_htlcs, and pending_intercepted_htlcs from Channel data, which will soon be included in the ChannelMonitors as part of a different series of PRs. We also fix the reload_node test util to use the node's pre-reload config after restart. The previous behavior was a bit surprising and led to one of this commit's tests failing.

We have an overarching goal of (mostly) getting rid of ChannelManager persistence and rebuilding the ChannelManager's state from existing ChannelMonitors, due to issues when the two structs are out-of-sync on restart. The main issue that can arise is channel force closure. In the previous commit we started this process by rebuilding ChannelManager::decode_update_add_htlcs, forward_htlcs, and pending_intercepted_htlcs from the Channel data, which will soon be included in the ChannelMonitors as part of a different series of PRs. Here we test that HTLC forwards that were originally received on 0.2 can still be successfully forwarded using the new reload + legacy handling code that will be merged for 0.3.

valentinewallace · 2025-12-03T23:37:02Z

I think the sermver CI failure is spurious

valentinewallace commented Nov 19, 2025

View reviewed changes

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

valentinewallace force-pushed the 2025-10-reconstruct-mgr-fwd-htlcs branch from 0b4eb68 to ce2ccac Compare November 19, 2025 21:56

valentinewallace force-pushed the 2025-10-reconstruct-mgr-fwd-htlcs branch from ce2ccac to 1e86521 Compare November 19, 2025 22:03

joostjager reviewed Nov 20, 2025

View reviewed changes

valentinewallace added this to Weekly Goals Nov 20, 2025

Git-ignore lightning-tests/target

4561bc5

Similar to the other /target directories we ignore where a bunch of files are generated during testing.

valentinewallace force-pushed the 2025-10-reconstruct-mgr-fwd-htlcs branch from 1e86521 to 26c6dc7 Compare December 2, 2025 00:51

valentinewallace force-pushed the 2025-10-reconstruct-mgr-fwd-htlcs branch from 26c6dc7 to ad79155 Compare December 2, 2025 23:00

valentinewallace marked this pull request as ready for review December 2, 2025 23:01

ldk-reviews-bot requested a review from jkczyz December 2, 2025 23:02

valentinewallace requested review from joostjager and removed request for jkczyz December 2, 2025 23:03

valentinewallace force-pushed the 2025-10-reconstruct-mgr-fwd-htlcs branch from ad79155 to 6b919ce Compare December 2, 2025 23:27

valentinewallace added 2 commits December 2, 2025 17:16

valentinewallace force-pushed the 2025-10-reconstruct-mgr-fwd-htlcs branch from 6b919ce to 0b83e80 Compare December 3, 2025 01:19

joostjager reviewed Dec 3, 2025

View reviewed changes

valentinewallace added 2 commits December 3, 2025 13:53

valentinewallace added 4 commits December 3, 2025 13:53

Tweak pending_htlc_intercepts ser on manager read

1a75d90

Makes an upcoming commit cleaner

valentinewallace force-pushed the 2025-10-reconstruct-mgr-fwd-htlcs branch from 0b83e80 to dfef41a Compare December 3, 2025 23:23

valentinewallace requested a review from joostjager December 3, 2025 23:53

Reconstruct ChannelManager forwarded HTLCs maps from Channels #4227

Are you sure you want to change the base?

Reconstruct ChannelManager forwarded HTLCs maps from Channels #4227

Conversation

valentinewallace commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

valentinewallace commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valentinewallace commented Nov 19, 2025

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace commented Dec 2, 2025

Uh oh!

tnull commented Dec 2, 2025

Uh oh!

valentinewallace commented Dec 2, 2025

Uh oh!

tnull commented Dec 3, 2025

Uh oh!

tnull commented Dec 3, 2025

Uh oh!

joostjager left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reconstruct `ChannelManager` forwarded HTLCs maps from `Channel`s #4227

Reconstruct `ChannelManager` forwarded HTLCs maps from `Channel`s #4227

valentinewallace commented Nov 17, 2025 •

edited

Loading

ldk-reviews-bot commented Nov 17, 2025 •

edited

Loading

codecov bot commented Nov 18, 2025 •

edited

Loading

valentinewallace commented Nov 19, 2025 •

edited

Loading

joostjager left a comment •

edited

Loading